[flang][OpenMP] Handle expressions in target ... do loop control
#107
Conversation
When emitting the ops required to compute the target loop's trip count, flang might emit ops outside the target regions that operands defined inside the region. This is fixed up by `HostClausesInsertionGuard` already. However, the current support only handles simple cases. If the loop control contains more elaborate expressions, the fix up logic does not handle it properly. This PR handles such cases.
ergawy
force-pushed
the
extend_fixup_logic
branch
from
a47b293
to
55235b9
Compare
| operand->getOwner()->setOperand(operand->getOperandNumber(), | ||
| lastSliceOp->getResult(operandResultIdx)); | ||
| } | ||
|
|
||
| auto useOutsideTargetRegion = [](mlir::OpOperand &operand) { |
There was a problem hiding this comment.
Now I'm wondering: Is everything from here to the end of the function needed still? It seems like your additions may potentially be already handling all cases. You can try commenting it out and running check-flang and smoke-fort tests and see what happens.
There was a problem hiding this comment.
Everything from this point below is still needed. What I added perpares for the following logic to work properly in case a target region argument is used indirectly outside the region. What I added above does not remap the target region arguments.
This is verifiable even using the lit test I added below. If you comment out the logic you previously added, you will find that now we clone the slice of operations needed by the trip count calculation but at the root of this slice, a use of the target region argument is still there.
There was a problem hiding this comment.
LGTM, I believe @skatrak covered everything with his comments! So please address them and wait on his approval prior to merging.
I have one likely stupid question though, as I am a little naive with this segment of work, would this create additional unused omp.map.info operations as we previously had the defining operation inside of the target region and then lifted it out? I would imagine not, and the inverse is more likely. But I thought it was worth clarifying my understanding of the interaction as we have a segment of code that will clone the use chain into the target region if it can, but if it can't it'll generate map operations. However, this is sort of the inverse situation in a way.
TL;DR: there's two ways we generate map.info operations implicitly, I am wondering if we end up generating map.info in this situation that we no longer require after this modification is performed!
Not a stupid question at all 😛! We do not create any additional |
Thank you for explaining that and clarifying it for me! |
When emitting the ops required to compute the target loop's trip count, flang might emit ops outside the target regions that operands defined inside the region. This is fixed up by
HostClausesInsertionGuardalready.However, the current support only handles simple cases. If the loop control contains more elaborate expressions, the fix up logic does not handle it properly. This PR handles such cases.